Feature/mvcombine #5025
Merged
Feature/mvcombine #5025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
srikanthpadakanti
requested review from
GumpacG,
LantaoJin,
MaxKsyunz,
RyanL1997,
Swiddis,
YANG-DB,
Yury-Fridlyand,
acarbonetto,
anirudha,
dai-chen,
derek-ho,
forestmvey,
joshuali925,
kavithacm,
mengweieric,
noCharger,
penghuo,
ps48,
qianheng-aws,
seankao-az,
vamsimanohar,
ykmr1224 and
yuancu
as code owners
yuancu
reviewed
Jan 27, 2026
Collaborator
yuancu
left a comment
yuancu
left a comment
There was a problem hiding this comment.
Thanks for contributing! I left a few comments.
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMvCombineCommandIT.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMvCombineCommandIT.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Show resolved
Hide resolved
Signed-off-by: Srikanth Padakanti <[email protected]>
Contributor
Author
|
Addressed the comments and tested locally to verify the results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java`:
- Around line 3353-3360: The metadata NULL projection is using the
post-aggregate schema from relBuilder.peek() which misindexes fields; in
CalciteRelNodeVisitor, capture and store the original input field types at the
start of visitMvCombine (e.g., a List of RelDataType or Type information from
the input row type) and pass that into restoreColumnOrderAfterArrayAgg; then
modify restoreColumnOrderAfterArrayAgg so when isMetadataField(fieldName) is
true it looks up the type from the saved original input types (instead of
relBuilder.peek().getRowType().getFieldList().get(i).getType()) and uses that
type for makeNullLiteral, ensuring projections are built from the original
schema.
In `@docs/user/ppl/cmd/mvcombine.md`:
- Line 60: Update the documentation descriptions so the dataset name matches the
queries by replacing references to "mvcombine" with "mvcombine_data" in the text
surrounding the examples (the occurrences around the lines that currently say
"Given a dataset mvcombine"), ensuring both places (the one at line ~60 and the
one at line ~92) use "mvcombine_data" to match the query source names.
- Line 117: Rename the heading text "Example 5: Error when field does not exist"
to "Example 4: Error when field does not exist" so example numbering is
sequential; locate the heading string in the mvcombine docs (the line containing
"Example 5: Error when field does not exist") and update it, then scan
subsequent example headings to ensure no other numbering gaps or off-by-one
issues.
- Line 58: The "Example 2" line is missing the Markdown header prefix and the
example numbering jumps (Example 3 -> Example 5); update the "Example 2" line to
include the same header style used for other examples (add "## " before "Example
2") and renumber the subsequent examples to maintain a sequential order (adjust
"Example 5" to "Example 4" or otherwise fix numbering so Example 1, Example 2,
Example 3, Example 4, etc.), referencing the visible example headings "Example
1", "Example 2", "Example 3", and "Example 5" to locate the spots to change.
In
`@integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMvCombineCommandIT.java`:
- Around line 195-203: Add missing null/boundary/invalid-input tests for the
mvcombine command: extend CalciteMvCombineCommandIT by adding new test methods
alongside testMvCombine_missingField_shouldReturn4xx that call executeQuery with
cases for null/empty input (e.g., "mvcombine " with no field or explicit null
values), boundary/empty collections, non-direct field references (e.g., function
or expression instead of field name) and already-multivalue fields; assert
proper 4xx/validation responses or expected behavior via ResponseException
status checks and positive assertions where applicable; use the existing pattern
in testMvCombine_missingField_shouldReturn4xx and reference executeQuery and
mvcombine to locate where to add tests.
- Around line 122-149: The current test
testMvCombine_multipleGroups_producesOneRowPerGroupKey uses loose OR checks on
mv0/mv1 which can hide grouping regressions; tighten it by asserting the sorted
row IPs explicitly (inspect result -> datarows -> each row's ip column) to
ensure row 0 is "10.0.0.7" and row 1 is "10.0.0.8", then validate the mv
contents per group deterministically (check mv0 contains the expected set for
10.0.0.7 and mv1 contains the expected set for 10.0.0.8) instead of the current
OR-based assertions—update references in this test to use the ip values from
r0.get(0)/r1.get(0) and assert exact membership of mv0/mv1 accordingly.
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMvCombineCommandIT.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteMvCombineCommandIT.java
Outdated
Show resolved
Hide resolved
added 4 commits
January 27, 2026 12:38
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
ykmr1224
reviewed
Jan 27, 2026
core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
Show resolved
Hide resolved
ykmr1224
reviewed
Jan 27, 2026
core/src/main/java/org/opensearch/sql/ast/analysis/FieldResolutionVisitor.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Signed-off-by: Srikanth Padakanti <[email protected]>
Contributor
Author
|
@ykmr1224 @anasalkouz |
LantaoJin
approved these changes
Jan 28, 2026
Contributor
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.19-dev 2.19-dev
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.19-dev
# Create a new branch
git switch --create backport/backport-5025-to-2.19-dev
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 92e73ead7c05c97668510de5cae246bdfdcff394
# Push it to GitHub
git push --set-upstream origin backport/backport-5025-to-2.19-dev
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.19-devThen, create a pull request where the |
Member
|
Thanks for your contribution, merged to main |
asifabashar
pushed a commit
to asifabashar/sql
that referenced
this pull request
Jan 28, 2026
* MvCombine Command Feature Signed-off-by: Srikanth Padakanti <[email protected]> * MvCombine Command Feature Signed-off-by: Srikanth Padakanti <[email protected]> * Add doctests to MvCombine Signed-off-by: Srikanth Padakanti <[email protected]> * spotlesscheck apply Signed-off-by: Srikanth Padakanti <[email protected]> * spotlesscheck apply Signed-off-by: Srikanth Padakanti <[email protected]> * spotlesscheck apply Signed-off-by: Srikanth Padakanti <[email protected]> * spotlessapply Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Add mvcombine to index.md Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * complete the checklist from ppl-commands.md Signed-off-by: Srikanth Padakanti <[email protected]> * spotlessApply Signed-off-by: Srikanth Padakanti <[email protected]> * Add visitMvCombine method to the FieldResolutionVisitor Signed-off-by: Srikanth Padakanti <[email protected]> * Apply spotlesscheck Signed-off-by: Srikanth Padakanti <[email protected]> * Add changes to exclude the metadata fields and remove the CAST logic Signed-off-by: Srikanth Padakanti <[email protected]> * Address CrossClusterSearchIT comment Signed-off-by: Srikanth Padakanti <[email protected]> * Address CrossClusterSearchIT comment Signed-off-by: Srikanth Padakanti <[email protected]> * Address CrossClusterSearchIT comment Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Address comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address comments Signed-off-by: Srikanth Padakanti <[email protected]> --------- Signed-off-by: Srikanth Padakanti <[email protected]> Signed-off-by: Srikanth Padakanti <[email protected]> Co-authored-by: Srikanth Padakanti <[email protected]> # Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java # ppl/src/main/antlr/OpenSearchPPLParser.g4
asifabashar
pushed a commit
to asifabashar/sql
that referenced
this pull request
Jan 28, 2026
* MvCombine Command Feature Signed-off-by: Srikanth Padakanti <[email protected]> * MvCombine Command Feature Signed-off-by: Srikanth Padakanti <[email protected]> * Add doctests to MvCombine Signed-off-by: Srikanth Padakanti <[email protected]> * spotlesscheck apply Signed-off-by: Srikanth Padakanti <[email protected]> * spotlesscheck apply Signed-off-by: Srikanth Padakanti <[email protected]> * spotlesscheck apply Signed-off-by: Srikanth Padakanti <[email protected]> * spotlessapply Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address coderrabbit comments Signed-off-by: Srikanth Padakanti <[email protected]> * Add mvcombine to index.md Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * Remove the nomv related implementation as that command is still not yet implemented Signed-off-by: Srikanth Padakanti <[email protected]> * complete the checklist from ppl-commands.md Signed-off-by: Srikanth Padakanti <[email protected]> * spotlessApply Signed-off-by: Srikanth Padakanti <[email protected]> * Add visitMvCombine method to the FieldResolutionVisitor Signed-off-by: Srikanth Padakanti <[email protected]> * Apply spotlesscheck Signed-off-by: Srikanth Padakanti <[email protected]> * Add changes to exclude the metadata fields and remove the CAST logic Signed-off-by: Srikanth Padakanti <[email protected]> * Address CrossClusterSearchIT comment Signed-off-by: Srikanth Padakanti <[email protected]> * Address CrossClusterSearchIT comment Signed-off-by: Srikanth Padakanti <[email protected]> * Address CrossClusterSearchIT comment Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Coderrabbit issues Signed-off-by: Srikanth Padakanti <[email protected]> * Address comments Signed-off-by: Srikanth Padakanti <[email protected]> * Address comments Signed-off-by: Srikanth Padakanti <[email protected]> --------- Signed-off-by: Srikanth Padakanti <[email protected]> Signed-off-by: Srikanth Padakanti <[email protected]> Co-authored-by: Srikanth Padakanti <[email protected]> # Conflicts: # integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteExplainIT.java # ppl/src/main/antlr/OpenSearchPPLParser.g4
This was referenced Jan 29, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This change introduces the mvcombine PPL command, which combines values of a specified field across rows that are identical on all other fields in the current result set.
mvcombine behaves as a pipeline-level grouping operator: it collapses matching rows into a single row and aggregates the target field values into a multivalue field, while preserving all non-target fields.
Command
mvcombine [delim=""]
Arguments
field (required)
The field whose values will be combined.
delim (optional)
Delimiter to be used when rendering a single-value representation of the combined field.
Semantics
mvcombine operates on the current result set.
Rows are grouped where all fields except have identical values.
For each group:
Scope clarification
Related Issues
Resolves #4766
#4766
Check List
--signoffor-s.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.